Avoid double scan stacks in continuation for compact cases #16022

LinHu2016 · 2022-09-30T20:07:47Z

When virtual thread mounts to J9VMThread, the GC might scan java stack of the related continuation twice (one from
J9VMThread.currentContinuation during thread scanning, another one from the related live continuation Object).
Scan twice the same java stack during compaction fix-up might cause wrong double fixup and we also might need to both scan for concurrent case(there is no issue for double scan in concurrent collectors).
So we stop to scan the stack of the continuation Object, which is
mounted to the J9VMThread for STW collectors.

Signed-off-by: Lin Hu linhu@ca.ibm.com

LinHu2016 · 2022-10-03T16:25:13Z

personal build: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/14364/

LinHu2016 · 2022-10-03T16:26:15Z

@amicic and @dmitripivkine please review the changes, thanks

amicic · 2022-10-03T16:35:42Z

runtime/oti/VMHelpers.hpp

@@ -2058,12 +2058,12 @@ class VM_VMHelpers
 	}

 	static VMINLINE bool
-	needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject) {
+	needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject, bool bSTWScan = false) {


I'd rename this parameter to scanOnly(Dis?)Mounted. Callers will know when it's important to take that into consideration (and pass true/false accordingly). STW has no meaning in this context.

amicic · 2022-10-03T16:41:31Z

@tajila or @babsingh or @fengxue-IS, please confirm that we don't have an existing way to examine if Virtual thread is currently mounted or not (given a reference to continuation object) and that bMount flag in the struct is a reasonable way to go.

Also, do you care if flag toggling is done on VM side, rather than in GC callback? I think I slightly prefer it on VM side, for example, just after/before swapFieldsWithContinuation is called.

amicic · 2022-10-03T16:42:39Z

runtime/gc_base/modronapi.cpp

@@ -65,6 +70,12 @@ postDismountContinuation(J9VMThread *vmThread, j9object_t object)
 	/* Conservatively assume that via mutations of stack slots (which are not subject to access barriers),
 	 * all post-write barriers have been triggered on this Continuation object, since it's been mounted. */
 	vmThread->javaVM->memoryManagerFunctions->J9WriteBarrierBatch(vmThread, object);
+#if JAVA_SPEC_VERSION >= 19
+		J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, object);


Currious, under which circumstances this pointer could be NULL?

for this case continuation could not be NULL, change it as Assert_MM_true(NULL != continuation);

On the VM side, JVM_VirtualThreadUnmountEnd will set it to NULL during the last unmount of a virtual thread. During the last unmount, the virtual thread state is set to TERMINATED. RI collects such virtual threads. RI also collects virtual threads if their state is NEW. Currently, we set CONTINUATION_VMREF for a virtual thread from its constructor when its state is NEW. Will this prevent a virtual thread in state NEW to be collected? If so, CONTINUATION_VMREF should be set during the first call to Continuation.enterImpl. @fengxue-IS

iirc, the vmRef field of a continuation is set during constructor code (createContinuation), so even if a new continuation that never gets executed will still have the field set.

amicic · 2022-10-03T16:55:51Z

runtime/gc_glue_java/CompactSchemeFixupObject.cpp

@@ -73,7 +73,7 @@ MM_CompactSchemeFixupObject::fixupContinuationObject(MM_EnvironmentStandard *env
 	fixupMixedObject(objectPtr);
 	/* fixup Java Stacks in J9VMContinuation */
 	J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread();
-	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) {
+	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr, true)) {


We need a comment here, something like:
In sliding compaction we must fix slots exactly once. Since we will fixup stack slots of mounted Virtual threads later during root fixup, we will skip it during this heap fixup pass (hence passing true(false) for scanOnly(Dis)Mounted parameter)

amicic · 2022-10-03T17:32:25Z

runtime/oti/VMHelpers.hpp

@@ -2058,12 +2058,12 @@ class VM_VMHelpers
 	}

 	static VMINLINE bool
-	needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject) {
+	needScanStacksForContinuation(J9VMThread *vmThread, j9object_t continuationObject, bool scanOnlyDisMounted = false) {


lower case 'M' to be consistent with pre/postMountContinuation API

amicic · 2022-10-03T17:43:59Z

runtime/gc_glue_java/HeapWalkerDelegate.cpp

@@ -58,7 +58,9 @@ void
 MM_HeapWalkerDelegate::doContinuationObject(MM_EnvironmentBase *env, omrobjectptr_t objectPtr, MM_HeapWalkerSlotFunc function, void *userData)
 {
 	J9VMThread *currentThread = (J9VMThread *)env->getLanguageVMThread();
-	if (VM_VMHelpers::needScanStacksForContinuation(currentThread, objectPtr)) {
+	/* In sliding compaction we must fix slots exactly once. Since we will fixup stack slots of mounted Virtual threads later during root fixup,


As a minimum, the comment should be adjusted, since HeapWalker is a tool that can be used outside of sliding compaction/movement fixup.

But more importantly, why is this fix needed for heap walker!? Heap walker does not walk over roots (as it does in compact fixup).
Could it walk over the same object twice via Remember Set walk? But if it is so, that has no relevance with thread being mounted or not....

Am I missing something else here?

yes, you are right, we don't need scan only once protection here.

babsingh · 2022-10-03T19:07:36Z

please confirm that we don't have an existing way to examine if Virtual thread is currently mounted or not (given a reference to continuation object) and that bMount flag in the struct is a reasonable way to go.

#15183 (comment) specifies an approach for VThreadContinuation. Now, continuations can be independent and not be associated to a virtual thread. IMO, the bMount flag in the struct is a reasonable way.

do you care if flag toggling is done on VM side, rather than in GC callback? I think I slightly prefer it on VM side, for example, just after/before swapFieldsWithContinuation is called.

It should be toggled as close to swapFieldsWithContinuation (in the same method) to avoid an inconsistent state in case the virtual thread is suspended.

@tajila @fengxue-IS for confirmation.

amicic · 2022-10-03T19:42:36Z

I can see that GC is not consistent with VM terminology (and from what I can quickly see, JEP too) in use of dismount vs unmount. Since we are already touching most of the files where Dismount is used, we should fix this now and use Unmount, instead.

fengxue-IS · 2022-10-03T20:05:58Z

#15183 (comment) specifies an approach for VThreadContinuation. Now, continuations can be independent and not be associated to a virtual thread. IMO, the bMount flag in the struct is a reasonable way.

+1
Note here that VM's definition of mount/unmount can different from GC as GC only cares which stack object continuation->stackObject is pointing to). Given such condition, I prefer if we don't use the lambda hacks to process additional data even if continuation isn't independent (future JEP structured concurrency may also have this problem as it may use continuation separately of virtualthread).

do you care if flag toggling is done on VM side, rather than in GC callback? I think I slightly prefer it on VM side, for example, just after/before swapFieldsWithContinuation is called.

It should be toggled as close to swapFieldsWithContinuation (in the same method) to avoid an inconsistent state in case the virtual thread is suspended.

As long as the toggle is done inside the INL where VMAccess is acquired, I don't have any problems.

babsingh · 2022-10-03T21:05:57Z

bMount flag in the struct is a reasonable way to go.

Instead of a flag, we should store the J9VMThread on which the continuation is mounted. Otherwise, it should be NULL. This approach will allow us to simplify our implementation at some locations.

typedef struct J9VMContinuation {
    ...
    J9VMThread *carrierThread;
}

enterContinuation {
    ...
    VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation);
    continuation->carrierThread = currentThread;
    ...
}

yieldContinuation {
    ...
    VM_ContinuationHelpers::swapFieldsWithContinuation(currentThread, continuation);
    continuation->carrierThread = NULL;
    ...
}

isMounted {
   return (NULL != continuation->carrierThread);
}

amicic · 2022-10-03T21:13:10Z

Instead of a flag, we should store the J9VMThread on which the continuation is mounted. Otherwise, it should be NULL. This approach will allow us to simplify our implementation at some locations

Makes sense. Useful for debugging too.

amicic · 2022-10-04T14:56:18Z

runtime/oti/j9nonbuilder.h

@@ -4998,6 +4998,7 @@ typedef struct J9VMContinuation {
 	UDATA* j2iFrame;
 	struct J9JITGPRSpillArea jitGPRs;
 	struct J9VMEntryLocalStorage* oldEntryLocalStorage;
+	J9VMThread *carrierThread;


I know this line is a copy-paste from a discussion, but in this file '*' is kept next to the type (rather then next to the field) name.

dmitripivkine · 2022-10-04T18:51:32Z

runtime/gc_base/GCExtensions.hpp

@@ -197,6 +197,7 @@ class MM_GCExtensions : public MM_GCExtensionsBase {
 	UDATA freeSizeThresholdForSurvivor; /**< if average freeSize(freeSize/freeCount) of the region is smaller than the Threshold, the region would not be reused by collector as survivor, for balanced GC only */
 	bool recycleRemainders; /**< true if need to recycle TLHRemainders at the end of PGC, for balanced GC only */

+	bool disableConcurrentSupportForContinuation;


Would you please add annotation to this variable?

I actually don't want this debug flag into official builds. Please remove it from this PR, and perform all the testing relevant to it in local test environment.

amicic · 2022-10-04T20:36:16Z

@babsingh not sure if you are distracted with other work, but did you have any concerns, or I can start PR testing?

LinHu2016 · 2022-10-04T20:40:59Z

personal build:
https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/14393/

babsingh

Functionally, LGTM. Minor documentation and formatting nitpicks.

runtime/oti/VMHelpers.hpp

runtime/gc_vlhgc/WriteOnceCompactor.cpp

runtime/oti/VMHelpers.hpp

babsingh · 2022-10-04T21:08:48Z

not sure if you are distracted with other work, but did you have any concerns, or I can start PR testing?

GTG. ~~Please proceed with PR testing once the above feedback is addressed.~~

When virtual thread mounts to J9VMThread, the GC might scan java stack of the related continuation twice (one from J9VMThread.currentContinuation during thread scanning, another one from the related live continuation Object). Scan twice the same java stack during compaction fix-up might cause wrong double fixup and we also might need to both scan for concurrent case(there is no issue for double scan in concurrent collectors). so we stop to scan the stack of the continuation Object, which is mounted to the J9VMThread for STW collectors. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2022-10-04T22:29:49Z

Jenkins sanity test all jdk19

babsingh · 2022-10-04T22:33:36Z

Fixed typo.
jenkins test sanity all jdk19

amicic · 2022-10-04T22:34:16Z

Jenkins test sanity all jdk19

amicic · 2022-10-04T22:34:49Z

Jenkins compile win jdk8

babsingh · 2022-10-04T22:49:45Z

#16022 (comment) launched https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/2809/ which was cancelled.

@amicic The builds launched by you:

amicic · 2022-10-04T22:55:44Z

Yeah, that was a close race. We launched it within a same minute. Good that you cancelled one. Thanks!

amicic · 2022-10-05T13:37:19Z

The check report is confused and still showing some active jobs, while they are actually aborted. But all that are not aborted are green, as can be seen from here:

https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/2810/
https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/2811/

LinHu2016 changed the title ~~Avoid double scan stacks in continuation for compact case~~ Avoid double scan stacks in continuation for compact cases Sep 30, 2022

amicic reviewed Oct 3, 2022

View reviewed changes

amicic added comp:gc project:loom Used to track Project Loom related work labels Oct 3, 2022

LinHu2016 force-pushed the avoiddoublescan branch from da1d292 to 1885931 Compare October 3, 2022 17:28

amicic reviewed Oct 3, 2022

View reviewed changes

LinHu2016 force-pushed the avoiddoublescan branch from 1885931 to 1df5cdf Compare October 3, 2022 18:20

LinHu2016 force-pushed the avoiddoublescan branch from 1df5cdf to df44a89 Compare October 3, 2022 20:47

LinHu2016 force-pushed the avoiddoublescan branch 2 times, most recently from 5c85306 to ceaada8 Compare October 4, 2022 14:31

amicic approved these changes Oct 4, 2022

View reviewed changes

amicic requested a review from babsingh October 4, 2022 14:51

amicic reviewed Oct 4, 2022

View reviewed changes

LinHu2016 force-pushed the avoiddoublescan branch 3 times, most recently from df7ed2b to b35d48e Compare October 4, 2022 17:37

dmitripivkine reviewed Oct 4, 2022

View reviewed changes

LinHu2016 force-pushed the avoiddoublescan branch from b35d48e to e32436e Compare October 4, 2022 19:15

babsingh requested changes Oct 4, 2022

View reviewed changes

runtime/oti/VMHelpers.hpp Outdated Show resolved Hide resolved

runtime/gc_vlhgc/WriteOnceCompactor.cpp Outdated Show resolved Hide resolved

runtime/oti/VMHelpers.hpp Outdated Show resolved Hide resolved

LinHu2016 force-pushed the avoiddoublescan branch from e32436e to 304431e Compare October 4, 2022 21:59

amicic merged commit 52ab41f into eclipse-openj9:master Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid double scan stacks in continuation for compact cases #16022

Avoid double scan stacks in continuation for compact cases #16022

LinHu2016 commented Sep 30, 2022

LinHu2016 commented Oct 3, 2022

LinHu2016 commented Oct 3, 2022

amicic Oct 3, 2022

amicic commented Oct 3, 2022 •

edited

Loading

amicic Oct 3, 2022 •

edited

Loading

LinHu2016 Oct 3, 2022

babsingh Oct 3, 2022 •

edited

Loading

fengxue-IS Oct 3, 2022

amicic Oct 3, 2022

amicic Oct 3, 2022

amicic Oct 3, 2022 •

edited

Loading

LinHu2016 Oct 3, 2022

babsingh commented Oct 3, 2022

amicic commented Oct 3, 2022

fengxue-IS commented Oct 3, 2022

babsingh commented Oct 3, 2022

amicic commented Oct 3, 2022

amicic Oct 4, 2022

dmitripivkine Oct 4, 2022

amicic Oct 4, 2022

amicic commented Oct 4, 2022

LinHu2016 commented Oct 4, 2022

babsingh left a comment •

edited

Loading

babsingh commented Oct 4, 2022 •

edited

Loading

amicic commented Oct 4, 2022

babsingh commented Oct 4, 2022

amicic commented Oct 4, 2022

amicic commented Oct 4, 2022

babsingh commented Oct 4, 2022

amicic commented Oct 4, 2022

amicic commented Oct 5, 2022

Avoid double scan stacks in continuation for compact cases #16022

Avoid double scan stacks in continuation for compact cases #16022

Conversation

LinHu2016 commented Sep 30, 2022

LinHu2016 commented Oct 3, 2022

LinHu2016 commented Oct 3, 2022

Choose a reason for hiding this comment

amicic commented Oct 3, 2022 • edited Loading

amicic Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh commented Oct 3, 2022

amicic commented Oct 3, 2022

fengxue-IS commented Oct 3, 2022

babsingh commented Oct 3, 2022

amicic commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amicic commented Oct 4, 2022

LinHu2016 commented Oct 4, 2022

babsingh left a comment • edited Loading

Choose a reason for hiding this comment

babsingh commented Oct 4, 2022 • edited Loading

amicic commented Oct 4, 2022

babsingh commented Oct 4, 2022

amicic commented Oct 4, 2022

amicic commented Oct 4, 2022

babsingh commented Oct 4, 2022

amicic commented Oct 4, 2022

amicic commented Oct 5, 2022

amicic commented Oct 3, 2022 •

edited

Loading

amicic Oct 3, 2022 •

edited

Loading

babsingh Oct 3, 2022 •

edited

Loading

amicic Oct 3, 2022 •

edited

Loading

babsingh left a comment •

edited

Loading

babsingh commented Oct 4, 2022 •

edited

Loading